Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harden remote plugin installs #12915

Merged
merged 16 commits into from
May 8, 2024
Merged

Conversation

lbajolet-hashicorp
Copy link
Contributor

Since we're hardening what Packer is able to load locally when it comes to plugins, we need also to harden the installation process a bit.

While testing we noticed some remotes had published their plugins with version mismatches between the tag and the binary.
This was not a problem in the past, as Packer did not care for this, only the binary name was important, and the plugin could be installed without problem.

Nowadays however, since Packer enforces the plugin version reported in the name to be the same as the plugin self-reported version, this makes it impossible for the installed plugin to load anymore in such an instance.

Therefore in order to limit confusion, and so users are able to understand the problem and report it to the plugins with that mismatch, we reject the installations that expose this mismatch, and report it to the user if they cannot install anything else.

@lbajolet-hashicorp lbajolet-hashicorp added enhancement command/plugins Issues related to the plugins management commands labels Apr 10, 2024
@lbajolet-hashicorp lbajolet-hashicorp requested a review from a team as a code owner April 10, 2024 21:46
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the harden_remote_plugin_installs branch 4 times, most recently from 6f12837 to d6662c9 Compare April 12, 2024 18:47
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the harden_remote_plugin_installs branch from d6662c9 to 7a60d5e Compare April 17, 2024 16:29
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the harden_remote_plugin_installs branch from 7a60d5e to 540c5b6 Compare April 19, 2024 14:02
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the harden_remote_plugin_installs branch from 540c5b6 to 1729ed9 Compare April 23, 2024 19:28
@nywilken
Copy link
Contributor

So this change started because we enforce the version match when installing via packer plugins install --path. I can't stop but wonder if instead of failing on version mismatch (describe vs final binary) if we should instead warn the user of the mismatch and install the plugin as a dev version.

I say that because for a user wishing to test a new version of a binary, if there is a mismatch, I will have to open an issue on the parent repo and wait. Or I can build the binary with the correct version which then introduces a new step of where someone has to build a binary just to fix the version. Or I can rename the binary to match the version which may be off the next time I install. All options which will need to be documented. IMO any manual intervention will bring CI/CD to halt until addressed.

By forcing the version mismatch at local binary install I can see folks acting to it sooner. If we default the version to a dev version I also see people continuing to use the binary but can then decide what to do. They may be okay with the binary being a dev version, especially since required_versions supports dev version matching. What do you think?

@lbajolet-hashicorp
Copy link
Contributor Author

So this change started because we enforce the version match when installing via packer plugins install --path. I can't stop but wonder if instead of failing on version mismatch (describe vs final binary) if we should instead warn the user of the mismatch and install the plugin as a dev version.

I say that because for a user wishing to test a new version of a binary, if there is a mismatch, I will have to open an issue on the parent repo and wait. Or I can build the binary with the correct version which then introduces a new step of where someone has to build a binary just to fix the version. Or I can rename the binary to match the version which may be off the next time I install. All options which will need to be documented. IMO any manual intervention will bring CI/CD to halt until addressed.

By forcing the version mismatch at local binary install I can see folks acting to it sooner. If we default the version to a dev version I also see people continuing to use the binary but can then decide what to do. They may be okay with the binary being a dev version, especially since required_versions supports dev version matching. What do you think?

Hmmmm, I like the idea honestly. This may indeed be a good bridging solution which may motivate users to report the problem upstream while not completely blocking them.

I'll see what I can do to amend this PR to adopt this behaviour instead.

errs = multierror.Append(errs, err)
continue
}
if descVersion.Prerelease() != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is also adding hardening to the installation of prereleases, right? Should this check be after the check of versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-release remote installation is not supported at the moment, only local installation is through packer plugins install --path.
Though with Wilken's last remark, I guess this may change so that the version remotely installed could be a dev one, albeit with a warning that there's a mismatch.
But in any case, describing a constraint with -dev in the version, or installing pre-releases through packer plugins install without passing in a path is not a supported workflow for the moment.

@nywilken
Copy link
Contributor

I added this description to the CHANGELOG as a temporary entry.

core: Remote plugins installed containing an internal version number that differs from the version
number within the binary name can lead to confusion when tracking Packer plugin version 
information. To help track such deprecates in the plugin version, packer init and packer plugin install have been updated to install such plugins as dev versions (e.g. 1.0.0-dev). 
Users are encouraged to notify plugin maintainers of any version mismatches. https://github.com/hashicorp/packer/pull/12915

I think that for remote installation and local installation a version mismatch should not fail warn and treat the binary as a development binary. This will not break user flows who may have this issue now. In the future if we need to we can make it a hard error. Thoughts.

@nywilken
Copy link
Contributor

nywilken commented Apr 29, 2024

hmmm. It looks like the following situation in Packer today errors silently. I tried renaming a binary to have a different version and Packer errors saying the the provision was not found. Looking at the logs I found the message indicating why it was not loaded. But this was not straight forward.

Options{PluginDirectory:"/Users/dev/.packer.d/plugins", BinaryInstallationOptions:plugingetter.BinaryInstallatio
nOptions{APIVersionMajor:"5", APIVersionMinor:"0", OS:"darwin", ARCH:"amd64", Ext:"", Checksummers:[]plugingetter.C
hecksummer{plugingetter.Checksummer{Type:"sha256", Hash:(*sha256.digest)(0xc0001da180)}}, ReleasesOnly:false}}
2024/04/29 11:17:10 plugin "/Users/dev/.packer.d/plugins/github.com/sylviamoss/comment/packer-plugin-comment_v1.
0.0-dev_x5.0_darwin_amd64" reported version "1.0.0" while its name implies version "v1.0.0-dev", ignoring

@lbajolet-hashicorp
Copy link
Contributor Author

hmmm. It looks like the following situation in Packer today errors silently. I tried renaming a binary to have a different version and Packer errors saying the the provision was not found. Looking at the logs I found the message indicating why it was not loaded. But this was not straight forward.

Options{PluginDirectory:"/Users/dev/.packer.d/plugins", BinaryInstallationOptions:plugingetter.BinaryInstallatio
nOptions{APIVersionMajor:"5", APIVersionMinor:"0", OS:"darwin", ARCH:"amd64", Ext:"", Checksummers:[]plugingetter.C
hecksummer{plugingetter.Checksummer{Type:"sha256", Hash:(*sha256.digest)(0xc0001da180)}}, ReleasesOnly:false}}
2024/04/29 11:17:10 plugin "/Users/dev/.packer.d/plugins/github.com/sylviamoss/comment/packer-plugin-comment_v1.
0.0-dev_x5.0_darwin_amd64" reported version "1.0.0" while its name implies version "v1.0.0-dev", ignoring

Ah yes, this is in line with how listing installations worked previously.
Maybe we should print those errors out in the diag instead of just reporting that no plugin was found in cases like these, so something like how InstallLatest works, where we stockpile errors and print them out only if the operation failed.

I'm a bit skeptical of this approach to an extent as it may report too many errors and drown the problem a bit, but this would help in situations like the one you describe.

@lbajolet-hashicorp lbajolet-hashicorp force-pushed the harden_remote_plugin_installs branch from 3c043c1 to cd51a94 Compare April 29, 2024 20:12
@nywilken
Copy link
Contributor

While I understand the rationale for wanting to include the check. It is adding an additional amount of work and a bit of unknowns that I fear will become a hurdle for users. Our goal for predictable loading was to consolidate plugin loading into a single binary name and directory. This additional change seems a bit premature at this time. Especially since less than a handful of plugins are running into this case.

Once we add this behavior to an official release it would be hard to retract it if it is relied upon. Seeing as this has never been an issue, let's hold and revisit if and when it becomes a thing. The new HCP Packer version tracking will bring more visibility into the versions being used.

I suggest we revert the change where we require the version in the binary name to match the version in the describe command as the next step. #12888

@lbajolet-hashicorp lbajolet-hashicorp force-pushed the harden_remote_plugin_installs branch 3 times, most recently from c356041 to 08e42c7 Compare May 1, 2024 20:00
The zipfile containing the binaries we attempt to install from a remote
source is placed in the temporary directory of the host.
In general it is wiped automatically by the OS, but in some cases
(windows typically), it isn't.

To avoid cluttering the temporary directory, we clean-up after
ourselves, and remove the temporary zip file that we create when
attempting to install a plugin, regardless of it succeeding or not.
Given that calling the describe command on plugins and deserialising the
output as a plugin description is something done in multiple places in
the code, we factorise this operation so we don't need to copy/paste the
code around.
When installing a remote plugin, and after we've either successfully
installed a binary, or exhausted all the possible sources, we print a
final error message if nothing was reported.

However, given that errs is a pointer to a structure, and if no errors
were produced, the the error list could be nil, leading to the call to
`Len()' to crash Packer.

This is exceedingly rare as in general the code attempts to read
multiple sources from Github, and therefore we almost always get some
error reported, but while changing the function's code, I managed to
make it crash while removing/changing some error statements.

Therefore to avoid future surprises, we first check that `errs' is not
nil before invoking `Len()' on it, as no errors and no plugins installed
mean that something went wrong all the same.
Since we named the version from the getter `version', this means we have
a naming conflict inside the loop that attempts to install a versioned
candidate for a plugin, making it impossible to invoke something from
the go-version package.

Since we'll introduce a change that needs the latter capability, we must
either rename the local variable to something else than `version', or we
need to alias the package locally.

This commit implements the latter, opting to call the package goversion.
Whenever a Github release exposes an entry for another OS/arch
combination, this gets registered as an error, which in the event no
binary is compatible with the host's OS/arch, gets reported at the end
of the getter process.

While this is sound in theory, in practice we get the list of all the
combinations that don't match the host's, which is not something a
Packer user can act on, and might therefore be more confusing than
helping to solve the issue.

Therefore we opt in this commit to stop registering those cases as real
errors, and only log them as an INFO statement.
When listing installed plugins, we check that the plugin's reported
version through describe matches what's in the name of the file.

Doing do, we were parsing the same version string twice without
modifying it, which was not necessary, so this commit changes that.
Since we're hardening what Packer is able to load locally when it comes
to plugins, we need also to harden the installation process a bit.

While testing we noticed some remotes had published their plugins with
version mismatches between the tag and the binary.
This was not a problem in the past, as Packer did not care for this,
only the binary name was important, and the plugin could be installed
without problem.

Nowadays however, since Packer enforces the plugin version reported in
the name to be the same as the plugin self-reported version, this makes
it impossible for the installed plugin to load anymore in such an
instance.

Therefore in order to limit confusion, and so users are able to
understand the problem and report it to the plugins with that mismatch,
we instead install the plugin as a dev version, and report it to the
user so they can still use it, but are able to report it to the plugin
developers.
When packer init is invoked with a --force argument, but no --update, we
clamp the version to install based on the last one locally installed.

Doing this may however cause the constraint to always be false if the
latest available version of a plugin is a pre-release, as none of the
upstream constraints will match that.

Therefore this commit changes how the constraint is derived from the
local list of installations, so that only the last installation that
matches the original constraint will be used, and not a pre-release.
When installing a plugin from a remote source, we list the installed
plugins that match the constraints specified, and if the constraint is
already satisfied, we don't do anything.

However, since remote installation is only relevant for releases of a
plugin, we should only look at the installed releases of a plugin, and
not consider pre-releases for that step.

This wasn't the case before this commit, as if a prerelease version of a
commit (ex: 10.8.1-dev), and we try to invoke `packer init` with a
constraint on this version specifically, Packer would locate that
pre-release and assume it was already installed, so would silently
succeed the command and do nothing.

This isn't the expected behaviour as we should install the final release
of that plugin, regardless of any prerelease installation of the plugin.
So this commit fixes that by only listing releases, so we don't report
the plugin being already installed if a prerelease is what's installed.
When a checksum file for a release is downloaded and iterated upon to
find the compatible binary for a release, we used to log each
non-compatible entry in the logs.

This is noisy as we know there's going to be multiple entries that don't
match the host's os/arch, and there's no good reason to show those, so
we silence them.
@nywilken nywilken force-pushed the harden_remote_plugin_installs branch from 1a37f84 to 709fdcd Compare May 7, 2024 02:37
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved once #12953 has been merged in.

nywilken added 5 commits May 8, 2024 15:48
This change is an attempt to remove the need for additional temporary files, along with
calls to stat the temp files, to reduce the number of files being created, opened, and closed.
In addition to this change, the logic for falling back to a previous version if the highest matched version
is a pre-release has been removed. Instead we will assume that any prior versions will exhibit the same issue and
return immediately. A user can install the version manually if they will or they can modify their version constraint
to a properly released version.
```
~>  packer init mondoo_req.pkr.hcl
Failed getting the "github.com/mondoohq/cnspec" plugin:
error:
Remote installation of the plugin version 10.8.1-dev is unsupported.
This is likely an upstream issue with the 10.8.1 release, which should be reported.
If you require this specific version of the plugin, download the binary and install it manually.

packer plugins install --path '<plugin_binary>' github.com/mondoohq/cnspec

```
* Only create plugin directories if there is  potential plugin install
… a prerelease

* Refactor InstallError string messages
@lbajolet-hashicorp lbajolet-hashicorp merged commit 20345f9 into main May 8, 2024
11 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the harden_remote_plugin_installs branch May 8, 2024 20:02
Copy link

github-actions bot commented Jun 8, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
command/plugins Issues related to the plugins management commands enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants